Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address Book POC implementation #499

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Address Book POC implementation #499

merged 2 commits into from
Jan 15, 2021

Conversation

jm-clius
Copy link
Contributor

@jm-clius jm-clius commented Jan 7, 2021

This PR closes #498

It introduces a POC version of a Peer Store with Address Book, based on similar implementations in go and js.

The POC:

  • Implements a simple AddressBook API to add, get, set and delete multiaddresses for peers
  • Implements a simple PeerStore API to get and delete stored information for peers across all books (although the address book is the only one implemented so far)
  • Allows clients to add their own listeners to be notified of peer store events. Each listener contains a number of handlers that are triggered on changes in the peer store. For POC purposes, I have only added a handler for multiaddr changes on a peer.

Suggested next steps:

  1. Gain consensus both on general approach of using a Peer Store and specifics of implementation (e.g. how listeners work, API usability, etc.)
  2. Implement KeyBook, ProtoBook and rudimentary MetadataBook (likely in this order)
  3. POC integration of Peer Store(s) in other modules. WakuRelay is a possible candidate here to implement a local Peer Store until it's fully integrated elsewhere in nim-libp2p
  4. TBD integration of a peer store in nim-libp2p (e.g. by building a peer manager, discovery interface, etc.)

dryajov
dryajov reviewed Jan 7, 2021
View reviewed changes
libp2p/peerstore.nim Outdated

PeerStore* = ref object of RootObj
addressBook*: AddressBook
listeners: ref seq[EventListener]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you're planning on passing listeners as an argument you probably don't need a ref here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, make sense. Fixed in 29dcc33.

libp2p/peerstore.nim Outdated
addrChange: addrChange)

proc init*(T: type PeerStore): PeerStore =
var listeners: ref seq[EventListener]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to reiterate, you don't need a ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 29dcc33.

oskarth
oskarth approved these changes Jan 8, 2021
View reviewed changes
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantic wise looks reasonable to me! I'll defer to @dryajov re nim-libp2p specifics.

libp2p/peerstore.nim
# Listener and handler types #
##############################

AddrChangeHandler* = proc(peerId: PeerID, multiaddrs: HashSet[MultiAddress])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this communicating change per se (as in a delta) or is the set here the sum of all, existing and new, multiaddresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, this could have been clearer. It's not merely the delta, but communicates all known multiaddrs per peer whenever there's a change as per js-implementation.

@jm-clius jm-clius requested a review from dryajov January 8, 2021 08:45
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #499 (29dcc33) into master (b2ea5a3) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
- Coverage   86.23%   86.19%   -0.04%     
==========================================
  Files          49       49              
  Lines       11672    11672              
==========================================
- Hits        10065    10061       -4     
- Misses       1607     1611       +4
Impacted Files Coverage Δ
libp2p/protocols/pubsub/gossipsub.nim 78.41% <0.00%> (-0.72%) ⬇️
libp2p/crypto/rsa.nim 86.57% <0.00%> (+0.22%) ⬆️

@jm-clius
Copy link
Contributor Author

@dryajov any further changes from your side, or can this be merged?

dryajov
dryajov reviewed Jan 13, 2021
View reviewed changes
libp2p/peerstore.nim

AddrChangeHandler* = proc(peerId: PeerID, multiaddrs: HashSet[MultiAddress])

EventListener* = object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we need an object to encapsulate the event's callback, it doesn't cary any additional information and can be easily added as parameter to the callback later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I somehow missed this comment.

I see your point. The encapsulating "listener" was created to hold all event handlers that may be defined for other books (keyChange, protoChange, etc.) so that the Peer Store wouldn't need a separate collection for each type of handler. Can easily change to such an implementation though.

(Going to merge this for now to unstable and address this in future PR once more books are added.)

@dryajov
Copy link
Contributor

dryajov commented Jan 13, 2021 •
edited
Loading

@jm-clius I think this looks close enough to what's in js and go and I think this is a good start. I would not merge it to master right now tho and instead I will create an unstable branch where all new features/development should happen first - master should remain stable and only get hotfixes and stable features. Please hold off on merging for now I'll do that once I create the unstable branch.

@dryajov
Copy link
Contributor

dryajov commented Jan 13, 2021

@jm-clius I've added an unstable branch, please repoint this PR there.

@jm-clius jm-clius changed the base branch from master to unstable January 14, 2021 07:04
@jm-clius jm-clius requested a review from dryajov January 14, 2021 07:05
@jm-clius jm-clius merged commit d9f6b40 into unstable Jan 15, 2021
@jm-clius jm-clius deleted the feat/add-peerstore branch January 15, 2021 08:36
@jm-clius jm-clius mentioned this pull request Jan 15, 2021
Merged
dryajov added a commit that referenced this pull request Feb 8, 2021
@dryajov @jm-clius
Merge unstable (#518)
2658181 
* Address Book POC implementation (#499)

* Address Book POC implementation

* Feat/peerstore impl (#505)

Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskarth oskarth oskarth approved these changes

@dryajov dryajov Awaiting requested review from dryajov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proof of Concept: Peer Store with Address Book
3 participants
@jm-clius @dryajov @oskarth